Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for frozen objects #6590

Merged
merged 72 commits into from
Nov 29, 2019
Merged

Add support for frozen objects #6590

merged 72 commits into from
Nov 29, 2019

Conversation

cmelchior
Copy link
Contributor

@cmelchior cmelchior commented Aug 23, 2019

Closes #4291
Closes #1208 ( most of it. There is still an open question about schedulers)

Blocked by:

This PR adds support for frozen Realm objects. The major benefit of frozen objects is that they are not thread confined and can thus be accessed on any thread. This is especially important for most kinds of stream-based architectures (which is most of them at this point).

The trade-off is that the objects are then immutable (which is actually considered a positive for most reactive architectures on Android)

High level API

// Adding two new methods to all top level Realm objects: .freeze() and .isFrozen()
// Anything can be frozen: Realm, Results, Lists, Objects
Realm realm = Realm.getInstance(config);
Realm frozenRealm = realm.freeze();
RealmResults<Person> persons = realm.where(Person.class).findAll().freeze();
Person p = realm.where(Person.class).findFirst().freeze();
RealmList<Child> children = realm.where(Person.class).findFirst().getChildren().freeze();

// The RxJava factory will be changed so observables now emit frozen objects by default
Realm realm = Realm.getInstance(config);
realm.where(Person.class).findAllAsync().asObservable()
  .map((result, changeSet) -> {
    result.isFrozen() == true;    
  })

// It is still possible to navigate and query a frozen Realm just like a live one, 
// but all objects will be frozen
Realm realm = Realm.getInstance(config);
Realm frozenRealm = realm.freeze();
frozenRealm.where(Person.class).findAll().isFrozen() == true;

// Because freezing objects increase the chance of leaking versions and thus 
// disk space we should expose some logic around Cores tracking of versions. 
// I suggest throwing an exception if the number of active versions goes over 
// a certain number.
RealmConfiguration config = new RealmConfiguration.Builder()
  .maxNumberOfActiveVersions(20);
  .build();

Semantics

Freezing an object will freeze the entire object graph at that version, effectively turning it read-only. Doing this will also lift the thread-confined restriction on it.

These are still to be decided, but so far I'm envisioning the following

  • Starting a write transaction on a frozen Realm will throw an exception
  • Trying to add a change listener will throw an exception.
  • Trying to refresh a Realm in any way will throw an exception
  • Trying to write to a frozen object inside the live Realms write transaction will throw.
  • Doing liveRealm.copyToRealmOrUpdate(frozenObject) should work the same way as using unmanaged objects.

Open design questions

Realm.getLocalInstanceCount(config):
What are the semantics of Realm.getLocalInstanceCount(config) for frozen Realms?

For now, globalCount == LocalThreadCount for frozen Realms, since these Realms are not thread constrained.

What are the semantics if the live Realm spawning frozen Realms is closed?
There are conflicting interests in terms of making it easy to reason about a Realm files life cycle vs. the Java objects. I.e. it becomes difficult to close a Realm file if frozen objects cannot easily close their Realm.
]
I can think of two solutions: 1) When all live Realms across all thread are closed, this will also automatically close all frozen Realms or 2) Introduce something like Realm.closeFrozenRealms(config) which will do it manually. It is unclear what is better.

For now, frozen Realms will all be closed when the live Realm spawning them are. This is primarely driven by making RxJava support easier.

Freeze existing object or return frozen copy?
Should freezing objects return a frozen copy or freeze the object in place?

There are performance benefits to freezing current object, but it seems to conflict with e.g. RxJava support and people need access to the live object in order to get notifications. Kotlin Multiplatform freeze in place.

For now we return frozen copies. I suggest we leave it up to user feedback to see if we should change it. We can consider having an internal method that freezes in-place if it will improve performance a lot.

Equality and hashcode:
How should object equality work for frozen objects? Current RealmLists and RealmLists compare objects on pr. object basis. How should frozen Realms and objects be compared?

For now Frozen objects with the same values are not considered equal to a live object. Same for Realms. Frozen objects at different versions (but equal values) are also not considered equal.

Use cases are unclear. The suggestion is to leave it up to user feedback to see if we should change it.

Generic parameter on RealmModel?
So it becomes: RealmModel<MyImplementation>, e.g public class Dog extends RealmObject<Dog>.

This enables better return types on freeze() as well as the current asFlowable() (which is kinda annoying to use). Setting the parameter is optional in Java (non-breaking), but seems to be required in Kotlin if present.

Unclear if we want to break everyone using Kotlin for this, but it will remove a lot of annoying casts. I will defer changing this to a later PR so we can discuss further (and to prevent cluttering this PR too much).

Benchmark results (on Emulator)

Results where frozen with 10.000 objects. It looks like freezing TableViews are fast enough that we don't need to be too concerned about performance. For a more scalable solution we should look into freezing TableViews off the UI thread (but doing it on the notifier thread also seems like a bad idea).

benchmark:       149,500 ns EMULATOR_UNLOCKED_FrozenObjectsBenchmarks.freezeResults
benchmark:        27,400 ns EMULATOR_UNLOCKED_FrozenObjectsBenchmarks.freezeList
benchmark:        24,454 ns EMULATOR_UNLOCKED_FrozenObjectsBenchmarks.freezeObject
benchmark:        14,375 ns EMULATOR_UNLOCKED_FrozenObjectsBenchmarks.freezeRealm

TODO:

  • Changelog
  • Cleanup
  • Finalize Add support for frozen objects realm-object-store#856 and merge it to master
  • Implement RealmObject.freeze() and RealmObject.isFrozen()
  • Implement RealmModel.freeze() and RealmModel.isFrozen()
  • Implement RealmList.freeze() and RealmList.isFrozen()
    Implement RealmResults.freeze() and RealmResults.isFrozen()
  • Implement Realm.freeze() and Realm.isFrozen()
  • Implement DynamicRealm.freeze() and DynamicRealm.isFrozen()
  • It should be possible to freeze deleted objects (which might seem a bit weird, but is needed for us to support event streams that want to notify about deleted objects).
  • Fully closing the live Realm should also close all frozen instances
  • Add benchmarks for freezing Realm
  • Add benchmarks for freezing large RealmResults
  • Add benchmark for freezing large RealmList
  • Add benchmark for freezing RealmObject
  • Javadoc
  • Change RxObservableFactory so it returns frozen objects.
  • Add option to RealmConfiguration to throw at certain number of active versions. Should be checked when we start a write transaction.
  • All mutator methods should throw on frozen Realms
  • Tests: Starting write transactions fail
  • Tests: Adding Realm listeners fail
  • Tests: Adding Results listeners fail
  • Tests: Adding List listeners fail
  • Tests: Adding Object listeners fail
  • Tests: refreshing fail
  • Tests: Equality of frozen Realms
  • Tests: Equality of frozen RealmObject
  • Tests: copyOrUpdate frozen Realm data
  • Tests: insertOrUpdate frozen Realm data
  • Other tests

# Conflicts:
#	realm/realm-library/src/main/cpp/object-store
# Conflicts:
#	CHANGELOG.md
#	dependencies.list
#	realm/realm-library/src/androidTest/java/io/realm/RealmTests.java
#	realm/realm-library/src/androidTest/java/io/realm/internal/JNIQueryTest.java
#	realm/realm-library/src/main/cpp/object-store
#	version.txt
Table table = realm.getTable(PrimaryKeyAsString.class);
assertNotNull(OsObjectStore.getPrimaryKeyForObject(realm.getSharedRealm(), PrimaryKeyAsString.CLASS_NAME));
assertTrue(table.hasSearchIndex(table.getColumnKey("name")));
assertFalse(table.hasSearchIndex(table.getColumnKey("name")));
Copy link
Contributor

@Zhuinden Zhuinden Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary keys are not indexed by default anymore? 🤔 Or only on String primary keys?

Will people need to write a migration to handle this change? IIRC index changes are destructive, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Core 6 we are mapping user defined String primary keys to our internal ObjectId which is faster so the index isn't strictly needed for looking up single objects.

That said, it is possible that we are forgetting about some use cases or that we are missing some upgrade tests. Thanks for pointing it out 👏

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no migration needed, there's a test covering this use case in d113014#diff-fe5ca9f6e0254b639a10bcc000406fdbR1439

@cmelchior
Copy link
Contributor Author

The branch should now be up to date with the Core-6 branch and all test parses. @nhachicha

Copy link
Collaborator

@nhachicha nhachicha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's time to ship this to Core6 branch, nice job 👏

@cmelchior cmelchior merged commit 620b617 into nh/core-6 Nov 29, 2019
@cmelchior cmelchior deleted the cm/core6/frozen-objects branch November 29, 2019 09:44
nhachicha added a commit that referenced this pull request Feb 3, 2020
* WIP Core6 integration

* Update tests & OS

* Disable file download if custom core path is set

* Latest Core 12fd1cdd44eacf7cda7c9d5e8a5618f0704ba657 fixes some Sort disabled tests

* Updated disabled tests, with reference to their issue.

* Update reference to disabled test

* - Removed a test non relevant in Core6
- Update disabled test with reference to issue

* Update disabled test with reference to the issue

* Enabling a test (passing with new Core/OS 🎉)

* - Behaviour of stopWaitForChange changed in Core6 causing all waiting threads to be released, adapting tests accordingly & deprecated the usage of `stopWaitForChange` & `waitForChange`

* Disabled tests concerning nullability (issue with test logic and Core crash)

* Better kotlin tests

* Add WIP support for TableRef instead of Table

* Use TableRef instead of Table

* Remove TBL macro

* Reference open Java issue with regard to changes to fileformat upgrades.

* Use Cores set_nullability

* Updated core bug references

* Fixing library benchmark

* Enable tests after fixes in Core

* Re-enable test

* Re-enable more tests

* Fix library benchmark project so it can load in Android Studio

* Update test file and re-enable tests

* Update various tests & using latest Core-6 branch from OS

* - Removed usage of indices via colkey2spec_ndx
- Renamed indices based method to column key
- Updated OS commit to fix https://github.com/realm/realm-object-store/issues/678

* Fixes to upgrade to origin/master

* Parse gradle root level properties to sub projects

* - Enabling more tests, now all tests passes, sync integration tests are still failing because of https://github.com/realm/realm-object-store/issues/848

* update OS commit

* Adapt to OS being behind master

* - Applying disabled C++ compile flag
- Cleanup

* Test are passing

* - Update dependencies
- Rename ROW to OBJ

* Update OS commit

* fixed PMD rule

* reverting check on valid ColKey since `bool(ColKey(-1))` returns true instead of false

* - Disabling Sync iNtegration Flaky Tests

* Making sure TableRef is used instead of Table*

* More usage of TableRef vs Table*

* remove OsObjectBuilder#maxColumnIndex() &  ObjectSchemaInfo#getMaxColumnIndex()

* Updated the annotation processor naming to use ColKey instead of index

* Enabling/rewrote indicies based tests again

* Remove usage of TR_ENTER_PTR

* - Renaming Table#isAttached to Table#isValid
-  Clearing TODO/FIXME

* Re-add missing benchmark files

* Newlines

* Cleanup & update OS Core6 branch

* Update OS

* Update to latest OS/Core

* Disabling failing test on CI

* Update to origin/master & uses a temporary OS branch with manual sync session creation commit cherry-picked

* Update to latest OS/Core

* Adding missing file

* Fix flaky test

* Added a test to check the Core6 no automatic index addition on String PK on an existing Core5 file

* adding missing file

* Add support for frozen objects (#6590)

* PR feedback
Cleanup & update to latest Core/OS versions

* Annotation processor: using ColKey instead of index

* PR feedback

* Bumped minSDK

* Update AP test

* Fix transform crashing when you delete transformed objects (#6680)

* Add multi-threaded stress test (#6677)

* Add multi-threaded stress set

* PR feedback. Use thread pool with variable amount of threads for reuse.

* Better support for Gradle offline mode (#6692)

* Update ReLinker (#6710)

* Upgrade to latest version of Sync and Core (#6723)

* Release v6.1.0

* Prepare next release v6.1.1-SNAPSHOT

* Prepare for next dev iteration

* cleanup

* update deps

* Updating tests

* Update OS commit

* Removed unused import

* Update CHANGELOG.md

Co-Authored-By: Christian Melchior <christian@ilios.dk>

* Update CHANGELOG.md

Co-authored-by: Christian Melchior <christian@ilios.dk>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants